Repository navigation
Add Azure Artifacts npm authentication refresh - #2339
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Adds cross-platform Azure Artifacts npm authentication setup for Microsoft contributors.
Changes:
- Adds guarded credential refresh and scoped
.npmrcgeneration. - Adds comprehensive Vitest coverage.
- Documents setup and ignores generated configurations.
Show a summary per file
| File | Description |
|---|---|
scripts/npm-auth-refresh.mjs |
Implements configuration and authentication refresh. |
scripts/npm-auth-refresh.d.mts |
Declares script types. |
nodejs/test/npm-auth-refresh.test.ts |
Tests CLI and platform behavior. |
nodejs/package.json |
Adds the refresh command. |
CONTRIBUTING.md |
Documents contributor setup. |
.gitignore |
Ignores generated .npmrc files. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 5/6 changed files
- Comments generated: 2
- Review effort level: Balanced
SteveSandersonMS
left a comment
SteveSandersonMS
left a comment
There was a problem hiding this comment.
Line 32 — writeProjectNpmConfigs unconditionally overwrites .npmrc files, erasing any pre-existing unrelated settings. Should either merge the @github:registry key into existing content or reject if the file exists and wasn't generated by this script.
Line 63 — Non-Windows path missing -f/--force flag for artifacts-npm-credprovider. Without it, the documented 'rerun after Azure 401/403' recovery step won't force a fresh token refresh on mac/Linux.
Line 88 — CodeQL alert: shell command built from process.env.ComSpec. Either hardcode the interpreter or validate/sanitize the env value.
SteveSandersonMS
left a comment
SteveSandersonMS
left a comment
There was a problem hiding this comment.
Review of technical issues
|
The PR addresses a legitimate internal Azure Artifacts npm-auth need, but two bugs were independently reproduced and a third requires confirmation: existing The PR is now draft for tracking. Please address these items and mark it ready for review once the changes are complete. |
|
Rechecked PR #2339 since the prior pass: there are no new commits, issue comments, review comments, reviews, or author response. The PR remains Draft and blocked with the same unresolved issues. No code changed, so there was nothing new to retest manually. Please address the requested fixes and mark the PR ready for review when complete. |
Preserve local npm settings, force Unix credential refresh, and keep Windows command execution independent of ComSpec and repository path arguments. Document running the helper from the repository root. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: ee3a1493-bf0d-4e08-9e13-03b3999affe0
SDK Consistency ReviewReviewed the changed files in this PR:
This PR introduces a local developer-tooling script ( Since this is purely internal build/auth tooling scoped to the Node.js package's dev workflow (not a client-facing SDK feature), there is no cross-language consistency requirement here — no action needed for other SDKs.
|
SteveSandersonMS
left a comment
SteveSandersonMS
left a comment
There was a problem hiding this comment.
Confirmed the requested fixes are in place in 1022970: .npmrc updates now preserve unrelated settings while consolidating @github:registry, Windows no longer trusts ComSpec and uses cwd, and Unix auth now forces refresh with -f. Regression coverage for these scenarios is present and focused validation is green.
The remaining Java win32-arm64 required-check failure appears unrelated CI noise based on the run log; once that is rerun/cleared this should be good to merge.
Summary
@githubwhile storing credentials at user levelValidation
npm test -- npm-auth-refresh.test.ts(15 passed)npm run lint(passed with 3 existing warnings)npm run typechecknpx tsc --noEmit --strict --skipLibCheck --module NodeNext --moduleResolution NodeNext --target ES2022 --esModuleInterop test\npm-auth-refresh.test.tsnpx prettier --config .prettierrc.json --check test\npm-auth-refresh.test.ts ..\scripts\npm-auth-refresh.mjs ..\scripts\npm-auth-refresh.d.mtsnode .\scripts\npm-auth-refresh.mjs --helpnpm run format:checkwas also run; on this Windows checkout it reports existing CRLF formatting drift in 104 untouched Node files. All new auth files pass the targeted Prettier check above.